feat: Add theme switching and group client settings#72
feat: Add theme switching and group client settings#72alishair7071 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
Add theme selector Dark/Light in settings page Group Mouse Sensitivity, Invert Scroll, and Theme under 'Client Settings' section Persist theme to localStorage (rein-theme)
📝 WalkthroughWalkthroughThe pull request introduces theme switching functionality to the application. A ThemeInit component initializes the theme from localStorage on app startup, while the settings page gains a new "Client Settings" section with a theme selector dropdown, alongside mouse sensitivity and scroll inversion toggles, with all client settings persisted to localStorage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage as Settings Page
participant State as React State
participant Storage as localStorage
participant DOM
User->>SettingsPage: Select theme from dropdown
SettingsPage->>State: Update theme state
State->>Storage: Persist theme to localStorage
State->>DOM: Apply data-theme attribute
DOM->>DOM: Re-render with new theme styles
Note over SettingsPage,DOM: On app startup (via __root.tsx)
SettingsPage->>Storage: Read theme from localStorage
Storage-->>State: Initialize theme state
State->>DOM: Apply saved data-theme attribute
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/routes/settings.tsx`:
- Around line 234-238: The helper label currently rendering the invertScroll
helper text should be changed to a non-form label element (e.g., a <span> or
<p>) so it isn't treated as a form label, and the Theme label must be associated
with its control by adding htmlFor on the label and a matching id on the
<select> element (update the Theme label and the corresponding <select> in the
settings component, referencing the invertScroll helper text and the theme
<select> control by their existing JSX locations/names) — replace the decorative
<label> for invertScroll with a <span> or <p>, and add id="theme" (or another
unique id) to the <select> and htmlFor="theme" to the Theme <label>.
🧹 Nitpick comments (2)
src/routes/__root.tsx (1)
28-36: Theme may flash (FOUC) on page load.
useEffectruns after the first paint, so users may briefly see the default/unstyled theme beforedata-themeis applied. For a seamless experience, consider moving the theme initialization to a synchronous inline<script>in the<head>(before any body content renders) that readslocalStorageand setsdata-themeon<html>.That said, this is a minor UX polish item and fine to defer.
src/routes/settings.tsx (1)
15-30: Consider centralizing localStorage keys inAPP_CONFIG.
'rein_invert'(line 18) and'rein_sensitivity'(line 27) are hardcoded strings, while the theme key usesAPP_CONFIG.THEME_STORAGE_KEY. For consistency and easier refactoring, consider moving all localStorage keys intoAPP_CONFIG.
| <label className="label"> | ||
| <span className="label-text-alt opacity-50"> | ||
| {invertScroll ? 'Traditional scrolling enabled' : 'Natural scrolling'} | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
Accessibility: labels not associated with their controls.
The static analysis tool correctly flags these two <label> elements. The helper-text label at line 234 and the "Theme" label at line 242 lack htmlFor attributes and don't wrap their associated controls.
For the Theme label, add htmlFor pointing to the <select>, and give the select an id. The helper-text label for Invert Scroll is purely decorative — consider using a <span> or <p> instead of <label>.
Proposed fix
- <label className="label">
+ <span className="label">
<span className="label-text-alt opacity-50">
{invertScroll ? 'Traditional scrolling enabled' : 'Natural scrolling'}
</span>
- </label>
+ </span>- <label className="label">
+ <label className="label" htmlFor="theme-select">
<span className="label-text">Theme</span>
</label>
<select
className="select select-bordered w-full"
+ id="theme-select"
value={theme}
onChange={(e) => setTheme(e.target.value)}
>Also applies to: 242-244
🧰 Tools
🪛 Biome (2.3.14)
[error] 234-238: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
🤖 Prompt for AI Agents
In `@src/routes/settings.tsx` around lines 234 - 238, The helper label currently
rendering the invertScroll helper text should be changed to a non-form label
element (e.g., a <span> or <p>) so it isn't treated as a form label, and the
Theme label must be associated with its control by adding htmlFor on the label
and a matching id on the <select> element (update the Theme label and the
corresponding <select> in the settings component, referencing the invertScroll
helper text and the theme <select> control by their existing JSX
locations/names) — replace the decorative <label> for invertScroll with a <span>
or <p>, and add id="theme" (or another unique id) to the <select> and
htmlFor="theme" to the Theme <label>.
1 participant
Summary
Added theme switching functionality and grouped all client-side settings in a dedicated section below the Save Config button.
Fixes #70
Changes
Testing
Rein.Theme.Switching.mp4
Summary by CodeRabbit
New Features
Style